-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip Mode #6039
base: master
Are you sure you want to change the base?
Skip Mode #6039
Conversation
b43d296
to
d26c315
Compare
d378e3a
to
e9c2e25
Compare
3d002ee
to
dd0a9fc
Compare
0d42eab
to
c37ef72
Compare
3839a8a
to
d7ee653
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code getting close, small things:
0e69c40
to
700c477
Compare
Have started functional testing, looking good so far, a couple of small issues: 1) Cannot broadcast
|
Fixed by f2fb60e. It turned out to be a bit of a rabbit hole with disable task event handlers. I can see wanting to change this as being a reason to broadcast skip mode settings.
No, but I don't think it's my change at fault - Since it appears to exist as an entirely separate bug present in Cylc as far back as 8.0.0! I'll attempt to fix anyway! Proposed fix up at #6351 - since it's a bug. Will rebase and fix after.
Fixed 660ca3a I have noticed that |
969267e
to
2b395d0
Compare
b0e4d94
to
e208f54
Compare
e208f54
to
d354fad
Compare
} | ||
} | ||
id_ = flow(cfg) | ||
schd = scheduler(id_, run_mode=workflow_run_mode, paused_start=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This integration test is actually running real jobs!
However, I don't think it's actually testing that the run mode
is correctly overridden because it doesn't check how the task is submitted.
Also, it's testing live
and skip
modes, however, the workflow run modes are live
and simulation
?!
Here's my suggestion, it goes with the small task_job_mgr
change I posted above (makes it easier to suppress live jobs from running whilst still testing non-live submission):
from cylc.flow.task_state import TASK_STATUS_SUBMITTED, TASK_STATUS_SUCCEEDED
@pytest.fixture
def capture_live_submissions(capcall):
"""Capture live submission attempts.
This prevents real jobs from being submitted to the system.
If you call this fixture from a test, it will return a set of tasks that
would have been submitted had this fixture not been used.
"""
def fake_submit(self, _workflow, itasks, *_):
for itask in itasks:
for status in (TASK_STATUS_SUBMITTED, TASK_STATUS_SUCCEEDED):
self.task_events_mgr.process_message(
itask,
'INFO',
status,
'2000-01-01T00:00:00Z',
'(received)',
)
return itasks
# suppress and capture live submissions
submit_live_calls = capcall(
'cylc.flow.task_job_mgr.TaskJobManager._submit_live_task_jobs',
fake_submit
)
def get_submissions():
nonlocal submit_live_calls
return {
itask.identity
for ((_self, _workflow, itasks, *_), _kwargs) in submit_live_calls
for itask in itasks
}
return get_submissions
@pytest.mark.parametrize('workflow_run_mode', ['live', 'skip']) # TODO: Should this be 'simulation'?
async def test_run_mode_override_from_config(
capture_live_submissions, flow, scheduler, run, complete, workflow_run_mode
):
"""Test that `[runtime][<namespace>]run mode` overrides workflow modes."""
id_ = flow({
'scheduling': {
'graph': {
'R1': 'live & skip',
},
},
'runtime': {
'live': {'run mode': 'live'},
'skip': {'run mode': 'skip'},
}
})
schd = scheduler(id_, run_mode=workflow_run_mode, paused_start=False)
async with run(schd):
await complete(schd)
if workflow_run_mode == 'live':
assert capture_live_submissions() == {'1/live'}
else:
assert capture_live_submissions() == set()
Note: This test requires #6326 (because I was too lazy to add an itask.jobs
entry, but could do that also)
Could go further by mocking away the get_submission_method
method to test which of the dummy submission methods is called if desired.
Might be worth pulling in the workflow run modes in case they change too:
@pytest.mark.parametrize('workflow_run_mode', [mode.value for mode in WORKFLOW_MODES])
339367f
to
e2703ec
Compare
* Add `[runtime][<namespace>]run mode` and `[runtime][<namespace>][skip]`. * Spin run mode functionality into separate modules. * Run sim mode check with every main loop - we don't know if any tasks are in sim mode from the scheduler, but it doesn't cost much to check if none are. * Implemented separate job "submission" pathway switching. * Implemented skip mode, including output control logic. * Add a linter and a validation check for tasks in nonlive modes, and for combinations of outputs * Enabled setting outputs as if task ran in skip mode using `cylc set --out skip`. * Testing for the above. Schema: use `Enum` for task run mode instead of `String` (#61) * Schema: use `Enum` for task run mode instead of `String` * Tidy fixup merge fix broken functional test Improve cylc set --out skip * Improve documentation of feature in cylc set --help * Allow cylc set --out=skip,optional_output * Test the above Remove test: We don't want users opting out of validating [runtime][ns][simulation/skip] because we can now changes these in a running workflow. stop users opting out of validating workflows without validating ski/simulation taskdef sections added tests for db entries in nonlive mode ensure db entries for all four modes are correct. move the change file toi the correct name get localhost of platforms 'simulation' 'skip' or 'dummy' not defined. (They probably shouldn't be, but that's a site specific choice...) fix tests with extra messages surfaces by using log_filter make cylc clean and remote tidy not try to clean or tidy platforms stop dummy mode appearing to submit twice Prevent cleanup from attempting to remote clean platforms skip and simulation Update cylc/flow/run_modes/skip.py Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk> fix small issues from OS review response to review: Make satisfaction method correct according the proposal Response to review * Allow only run modes skip and live to be selectable in the config. * Disallow workflow run modes sim and dummy from being overridden. * Attach Run mode to task_proxy rather than the task def. Response to review * Allow only run modes skip and live to be selectable in the config. * Disallow workflow run modes sim and dummy from being overridden. * Attach Run mode to task_proxy rather than the task def. don't run sim time check unless workflow in sim mode test that skip mode is only applied to a single task. remove now illegal items from test Response to review: - Remove Workflow Mode for tasks and make them default to live. - Ensure that we are checking (assert) log_filters. - Remove need for polling in functional test. :) usin enums Apply suggestions from code review Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
df593a7
to
df803d0
Compare
Closes #5641 (also fixes #5975, and fixes #5820 )
Skip Mode Proposal Doc
This branch includes (marked against skip mode proposal):
[runtime][<namespace>]run mode
.a. Broadcast can change
run mode
for future task job submissions.b. Cylc Validate and lint will warn about the setting not being live.
cylc set --out skip
sets outputs from skip mode.run mode = skip
respectsis_held
flag.Extras
7. Run Mode is available as an task attribute in the UI
8. When tasks are run in skip mode, the prerequisites which correspond to the outputs they generate should be marked as satisfied by skip mode rather than satisfied naturally for provenance reasons. For the purpose of cylc remove logic, satisfied by skip mode should be treated the same as satisfied naturally.
There are two extensions, which I haven't dealt with yet, because I want to ensure that the basic functionality works, and move to the substantial documentation PR which need follow this.
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users